Skip to content
This repository has been archived by the owner on Jan 24, 2024. It is now read-only.

Adjust db struct to make faster and clearer #59

Merged
merged 11 commits into from
Jun 13, 2018

Conversation

guochaorong
Copy link
Collaborator

No description provided.

@guochaorong guochaorong requested a review from Superjomn May 30, 2018 16:43
@guochaorong guochaorong mentioned this pull request May 31, 2018
web/api.py Outdated
commit_ids = []
for commit in commits:
if commit['commitid'] not in commit_ids:
commit_ids.append(commit['commitid'])
Copy link
Collaborator

@Superjomn Superjomn Jun 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

注释里是

returns: list of CommitRecord

函数名应该叫 get_all,不需要单独取 commit_id

返回值应该是 [CommitRecord]

web/api.py Outdated
commit_ids.append(commit['commitid'])
return commit_ids

def query_a_commit_info(self):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get_commit_record(self)

Copy link
Collaborator

@Superjomn Superjomn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

感觉差的比较多,之前好像一起定过接口,具体字段没有问题,但不需要暴露这么多接口。

下面的几个 method 就可以了。

class CommitRecord:
    @staticmethod
    def get_all(): 
        ''' Return list of CommitRecord '''
        pass

    def get_all_records(self):
         ''' Return list of TaskRecord '''
         pass

class TaskRecord:
    pass

class KpiRecord:
    pass

用户使用案例

  1. 获取所有的 commitid
commits = CommitRecord.get_all()
commitids = [c.commitid for c in commits]
  1. 获取一个 commitid 下所有的 task 记录
# some commit
commit = commits[k]
task_records = commit.get_all_records()
  1. 遍历一个 commit 下所有的 kpi record
for task_record in commit.get_all_records():
    for kpi_record in task_record.kpis:
        print(kpi_record.values)

web/api.py Outdated
'commitid': self.commit})

@staticmethod
def query_all_commit_infos():
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个函数不需要,用户可以直观用

commit_records = CommitRecord.get_all()
# if want to get all commit id
commit_ids = [c.commit_id for c in commit_records]

web/api.py Outdated
'commitid': self.commitid, 'task': self.name})

@staticmethod
def get_tasks_from_details(commit):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

不需要暴露 details

web/api.py Outdated
taskobj.kpis = taskobj.get_kpi_details()
res[taskobj.name] = taskobj
return res

def get_kpi_details(self):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个函数是不是private 就可以了,用户需要显式调用吗

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

脚本外目前没有调用的。就是拿1个task 的 kpi infos。
returns dict of KpiRecord
keys equal to kpi name,
values equal to KpiRecord'''

现在在commitRecord 类里面调用了这个函数, 改个名字 get_kpis? 是不是可以暴露出去

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

一个 TaskRecord 应该直接包含多个 KpiRecord,这个接口应该不需要额外调,所以不一定需要

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

嗯啊 这是个现有鸡还是先有蛋的问题, 如果他仅仅初始化了一个TaskRecord 对象, 还没调用任何方法, 这个对象还是空的。

现在在CommitRecord 这个类里面用到了TaskRecord 里面的这个方法, 如果写成私有的CommitRecord类还能调用到它么

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

感觉改个更general的名字, 比如get_kpis, 然后作为public 方法看着还行?

Copy link
Collaborator

@Superjomn Superjomn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@guochaorong guochaorong merged commit c5844b5 into PaddlePaddle:develop Jun 13, 2018
@guochaorong guochaorong deleted the db_new branch June 23, 2018 02:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants